Introduce TransportGetFromTranslogAction by pxsalehi · Pull Request #95998 · elastic/elasticsearch

pxsalehi · 2023-05-10T13:53:34Z

This is a change broken off from the ongoing real-time GET PR (#93976).
It is just an action that can be used to invoke the new ShardGetService.getFromTranslog
in #95736. It will be used on the search shards as a first step to handle a real-time get.

Relates #93976, ES-5537

elasticsearchmachine · 2023-05-10T13:53:58Z

Pinging @elastic/es-distributed (Team:Distributed)

pxsalehi · 2023-05-10T13:58:22Z

An earlier version of this has been already reviewed: https://github.com/elastic/elasticsearch/pull/93976/files#diff-8daf8bbe7449a3292228747c41ac167377916cc2ee31621eedfb56dd6d3465ee.

tlrx · 2023-05-11T07:04:51Z

server/src/main/java/org/elasticsearch/action/get/TransportGetFromTranslogAction.java

+        Engine engine = indexShard.getEngineOrNull();
+        if (engine == null) {
+            listener.onFailure(new AlreadyClosedException("engine closed"));
+            return;
+        }


I wonder if this is really needed because ShardGetService.getFromTranslog() uses getEngine() at some point and that will throw in case of a closed engine. I think we can do this only if the result of the getFromTranslog is null?

You're right, that's not needed there. Slightly outdated code! I've moved it to when result is null.

server/src/main/java/org/elasticsearch/action/get/TransportGetFromTranslogAction.java

tlrx · 2023-05-11T07:12:21Z

server/src/main/java/org/elasticsearch/action/get/TransportGetFromTranslogAction.java

+        private final GetRequest getRequest;
+        private final ShardId shardId;
+
+        public Request(GetRequest getRequest, ShardId shardId) {


GetRequest is a SingleShardRequest and can contain a ShardId, should we check that they are the same?

Indeed if it's the same shardId, then we can re-use the GetRequest as the request type of the TransportGetFromTranslogAction.

I did that in an initial version. See this comment. The code in SingleShardRequest is a bit old school. It keeps an internalShardId which is mutable and is only used internally. Essentially to reuse it, I'd have to make some internal mutable state of SingleShardRequest public which is not really worth it I think.

Thanks for pointing to the comment. I do agree SingleShardRequest did not aged well.

If I understand the code correctly, the internal shard id of the GetRequest is resolved when the request is executed through the TransportGetAction/TransportSingleShardAction, and TransportGetAction will be modified for realtime gets on stateless to use the new TransportGetFromTranslogAction and the resolved shard id will be passed over the the TransportGetFromTranslogAction.Request.

So we should be fine.

server/src/main/java/org/elasticsearch/action/get/TransportGetFromTranslogAction.java

tlrx · 2023-05-11T07:21:20Z

server/src/main/java/org/elasticsearch/action/get/TransportGetFromTranslogAction.java

+            segmentGeneration = in.readZLong();
+            if (in.readBoolean()) {
+                getResult = new GetResult(in);
+            }


Should we assert that segmentGeneration > -1L when getResult is null?

null and -1 is a valid state. I have mention that in the comment above segmentGeneration(), when we don't find anything and there hasn't been any unsafe->safe switches.

server/src/main/java/org/elasticsearch/action/get/TransportGetFromTranslogAction.java

tlrx · 2023-05-11T07:25:01Z

server/src/internalClusterTest/java/org/elasticsearch/get/GetFromTranslogActionIT.java

+        assertThat(response.getResult().isExists(), equalTo(true));
+        assertThat(response.segmentGeneration(), equalTo(-1L));
+        // After a refresh we should not be able to get from translog
+        client().admin().indices().refresh(new RefreshRequest("test")).get();


nit: refresh("test")

kingherc

Nice! Just some minor stuff I believe

server/src/internalClusterTest/java/org/elasticsearch/get/GetFromTranslogActionIT.java

server/src/main/java/org/elasticsearch/action/get/TransportGetFromTranslogAction.java

kingherc · 2023-05-11T09:26:19Z

server/src/main/java/org/elasticsearch/action/get/TransportGetFromTranslogAction.java

+        private final GetRequest getRequest;
+        private final ShardId shardId;
+
+        public Request(GetRequest getRequest, ShardId shardId) {


Indeed if it's the same shardId, then we can re-use the GetRequest as the request type of the TransportGetFromTranslogAction.

server/src/main/java/org/elasticsearch/action/get/TransportGetFromTranslogAction.java

kingherc · 2023-05-11T09:31:23Z

server/src/main/java/org/elasticsearch/action/get/TransportGetFromTranslogAction.java

+        public void writeTo(StreamOutput out) throws IOException {
+            out.writeZLong(segmentGeneration);
+            if (getResult == null) {
+                out.writeBoolean(false);


Do we really need to write a boolean? If segmentGeneration != -1, then the result is null and you simply do not need to write anything else.

kingherc · 2023-05-11T09:34:14Z

server/src/internalClusterTest/java/org/elasticsearch/get/GetFromTranslogActionIT.java

+        client().prepareIndex("test").setId("1").setSource("field1", "value1").setRefreshPolicy(RefreshPolicy.NONE).get();
+        response = getFromTranslog(indexOrAlias(), "1");
+        assertNotNull(response.getResult());
+        assertThat(response.getResult().isExists(), equalTo(true));


Shouldn't we also assert the real getResult's value? That e.g. it's equal to a document with field1&value1? Similar comment for the similar points below.

I'm not sure why that would matter for this case.

I think it is worth verifying that we are getting the right version of the document from the translog

server/src/internalClusterTest/java/org/elasticsearch/get/GetFromTranslogActionIT.java

…etFromTranslogAction

pxsalehi

Thanks for the reviews. It's ready now again.

pxsalehi · 2023-05-11T14:27:52Z

server/src/main/java/org/elasticsearch/action/get/TransportGetFromTranslogAction.java

+        Engine engine = indexShard.getEngineOrNull();
+        if (engine == null) {
+            listener.onFailure(new AlreadyClosedException("engine closed"));
+            return;
+        }


You're right, that's not needed there. Slightly outdated code! I've moved it to when result is null.

pxsalehi · 2023-05-11T14:30:27Z

server/src/main/java/org/elasticsearch/action/get/TransportGetFromTranslogAction.java

+        private final GetRequest getRequest;
+        private final ShardId shardId;
+
+        public Request(GetRequest getRequest, ShardId shardId) {


I did that in an initial version. See this comment. The code in SingleShardRequest is a bit old school. It keeps an internalShardId which is mutable and is only used internally. Essentially to reuse it, I'd have to make some internal mutable state of SingleShardRequest public which is not really worth it I think.

server/src/main/java/org/elasticsearch/action/get/TransportGetFromTranslogAction.java

pxsalehi · 2023-05-11T15:54:41Z

server/src/main/java/org/elasticsearch/action/get/TransportGetFromTranslogAction.java

+            segmentGeneration = in.readZLong();
+            if (in.readBoolean()) {
+                getResult = new GetResult(in);
+            }


null and -1 is a valid state. I have mention that in the comment above segmentGeneration(), when we don't find anything and there hasn't been any unsafe->safe switches.

server/src/main/java/org/elasticsearch/action/get/TransportGetFromTranslogAction.java

server/src/internalClusterTest/java/org/elasticsearch/get/GetFromTranslogActionIT.java

pxsalehi · 2023-05-11T16:01:56Z

server/src/internalClusterTest/java/org/elasticsearch/get/GetFromTranslogActionIT.java

+        client().prepareIndex("test").setId("1").setSource("field1", "value1").setRefreshPolicy(RefreshPolicy.NONE).get();
+        response = getFromTranslog(indexOrAlias(), "1");
+        assertNotNull(response.getResult());
+        assertThat(response.getResult().isExists(), equalTo(true));


I'm not sure why that would matter for this case.

tlrx

LGTM - I only left minor comments, feel free to address them

tlrx · 2023-05-12T07:06:07Z

server/src/internalClusterTest/java/org/elasticsearch/get/GetFromTranslogActionIT.java

+        client().prepareIndex("test").setId("1").setSource("field1", "value1").setRefreshPolicy(RefreshPolicy.NONE).get();
+        response = getFromTranslog(indexOrAlias(), "1");
+        assertNotNull(response.getResult());
+        assertThat(response.getResult().isExists(), equalTo(true));


I think it is worth verifying that we are getting the right version of the document from the translog

tlrx · 2023-05-12T07:09:49Z

server/src/internalClusterTest/java/org/elasticsearch/get/GetFromTranslogActionIT.java

+        assertNotNull(response.getResult());
+        assertThat(response.getResult().isExists(), equalTo(true));
+        assertThat(response.segmentGeneration(), equalTo(-1L));
+        // Get followed by a delete should still return a result


As a side note and for future tests, I think that comment like this are very helpful to understand test failures and can be backed into the assertion methods themselves, ie this could be:

assertThat("Get followed by a delete should still return a result", response.getResult(), nonNullValue());

Great idea!

server/src/main/java/org/elasticsearch/action/get/TransportGetFromTranslogAction.java

tlrx · 2023-05-12T08:02:42Z

server/src/main/java/org/elasticsearch/action/get/TransportGetFromTranslogAction.java

+        private final GetRequest getRequest;
+        private final ShardId shardId;
+
+        public Request(GetRequest getRequest, ShardId shardId) {


Thanks for pointing to the comment. I do agree SingleShardRequest did not aged well.

If I understand the code correctly, the internal shard id of the GetRequest is resolved when the request is executed through the TransportGetAction/TransportSingleShardAction, and TransportGetAction will be modified for realtime gets on stateless to use the new TransportGetFromTranslogAction and the resolved shard id will be passed over the the TransportGetFromTranslogAction.Request.

So we should be fine.

tlrx · 2023-05-12T08:05:00Z

server/src/main/java/org/elasticsearch/action/get/TransportGetFromTranslogAction.java

+
+        @Override
+        public String toString() {
+            return "GetFromTranslogRequest{" + "getRequest=" + getRequest + ", shardId=" + shardId + "}";


Maybe just

Suggested change

return "GetFromTranslogRequest{" + "getRequest=" + getRequest + ", shardId=" + shardId + "}";

return "translog " + getRequest;

as GetRequest.toString already provides useful information?

doesn't seem to include the shardID though.

server/src/main/java/org/elasticsearch/action/get/TransportGetFromTranslogAction.java

kingherc

LGTM2. Feel free to consider the remaining unresolved comments.

…FromTranslogAction.java Co-authored-by: Tanguy Leroux <tlrx.dev@gmail.com>

…gAction' into ps230509-transportGetFromTranslogAction

…etFromTranslogAction

pxsalehi · 2023-05-16T13:31:44Z

@elasticmachine run elasticsearch-ci/part-2

pxsalehi · 2023-05-16T13:35:56Z

Failure was #96162

pxsalehi · 2023-05-16T14:59:49Z

@elasticmachine update branch

pxsalehi · 2023-05-16T16:31:27Z

@elasticmachine run elasticsearch-ci/part-3

Failure is #95983

The multi-get counterpart of #95998. Relates ES-5677

pxsalehi added 2 commits May 10, 2023 13:28

first draft

4cc981f

clean up

7fe8dce

pxsalehi added >non-issue :Distributed/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. labels May 10, 2023

elasticsearchmachine added v8.9.0 Team:Distributed Meta label for distributed team. labels May 10, 2023

pxsalehi requested review from Tim-Brooks, kingherc and tlrx May 10, 2023 13:58

tlrx reviewed May 11, 2023

View reviewed changes

kingherc reviewed May 11, 2023

View reviewed changes

pxsalehi added 2 commits May 11, 2023 18:04

address review comments

20dad06

Merge remote-tracking branch 'upstream/main' into ps230509-transportG…

63c56aa

…etFromTranslogAction

pxsalehi commented May 11, 2023

View reviewed changes

pxsalehi requested review from kingherc and tlrx May 11, 2023 16:07

tlrx approved these changes May 12, 2023

View reviewed changes

kingherc approved these changes May 12, 2023

View reviewed changes

pxsalehi and others added 5 commits May 16, 2023 14:28

Update server/src/main/java/org/elasticsearch/action/get/TransportGet…

6c16445

…FromTranslogAction.java Co-authored-by: Tanguy Leroux <tlrx.dev@gmail.com>

Update server/src/main/java/org/elasticsearch/action/get/TransportGet…

469b316

…FromTranslogAction.java Co-authored-by: Tanguy Leroux <tlrx.dev@gmail.com>

address review comments

81aa75b

Merge remote-tracking branch 'origin/ps230509-transportGetFromTranslo…

1a2b1b6

…gAction' into ps230509-transportGetFromTranslogAction

Merge remote-tracking branch 'upstream/main' into ps230509-transportG…

00bea8f

…etFromTranslogAction

pxsalehi added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label May 16, 2023

Merge branch 'main' into ps230509-transportGetFromTranslogAction

3681e65

elasticsearchmachine merged commit fa264fb into elastic:main May 16, 2023

pxsalehi deleted the ps230509-transportGetFromTranslogAction branch May 16, 2023 17:07

pxsalehi mentioned this pull request Jun 1, 2023

Introduce TransportShardMultiGetFomTranslog #96492

Merged

pxsalehi added a commit that referenced this pull request Jun 9, 2023

Introduce TransportShardMultiGetFomTranslog (#96492)

2c1cd26

The multi-get counterpart of #95998. Relates ES-5677

	return "GetFromTranslogRequest{" + "getRequest=" + getRequest + ", shardId=" + shardId + "}";
	return "translog " + getRequest;

Conversation

pxsalehi commented May 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented May 10, 2023

Uh oh!

pxsalehi commented May 10, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kingherc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pxsalehi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlrx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pxsalehi commented May 10, 2023 •

edited

Loading

pxsalehi left a comment •

edited

Loading